fix(texlive-tags): tell a grep failure from zero matches - #175
Conversation
The audit discarded grep's standard error, so a grep that never ran produced the same empty result as one that found nothing, and the repository was reported as ok. A detector that answers "no drift" while broken is the failure this audit exists to avoid, and the same file already says so about cd: cd の失敗は握り潰さない。空の結果は「一致している」と区別が付かず、 何も見ていない監査が ok を出すことになる Exit codes cannot carry this: grep returns 1 when nothing matched, and xargs collapses any child status in 1..125 to 123, so "matched nothing" and "could not start" arrive as the same number. Standard error can -- grep writes nothing there when it simply found no match. Both greps now append to the existing err_file and one check after them raises the error, reusing the counter and ERROR line the clone failure already uses. Refs #173
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # 返すため、「一致しなかった」と「起動できなかった」が同じ値になる。 | ||
| # grep はマッチが無いだけなら標準エラーに何も書かないので、その有無で | ||
| # 判定する。捨ててしまうと、検出器が動いていない監査が ok を出す | ||
| : > "$err_file" |
There was a problem hiding this comment.
: > "$err_file" でファイルをクリアしてから、extra_patterns のループ内で 2>>"$err_file" と追記しています。この設計では、ループの途中のイテレーションで grep が失敗しても、後続のイテレーションが成功すれば err_file にはエラーが残り続けるという点は問題ありません。しかし逆に、最初の hits= の grep が失敗した場合でも、extra_patterns ループが全て正常終了すれば err_file にはエラーが書き込まれたまま残るため、正しく検出されます。
ただし、より深刻な問題として、err_file が複数のリポジトリ間で共有されている場合、前のリポジトリの grep エラーが残っていると誤検知が起きます。: > "$err_file" のクリアはループの先頭（各リポジトリの処理開始時）に置かれているため、この点は正しく対処されています。
一方で、err_file の定義箇所（スクリプト上部）を確認し、mktemp 等で一時ファイルとして作成されているか確認してください。もし固定パスであれば、並列実行時に競合が生じます。スクリプトが並列実行される可能性がある場合は mktemp による一時ファイルの利用を推奨します。
There was a problem hiding this comment.
確認しました。mktemp で作成されています。 固定パスではないので並列実行での競合はありません。
workdir=$(mktemp -d)
err_file=$(mktemp)
trap 'rm -rf "$workdir" "$err_file"' EXITtrap によるクリーンアップも入っています。ご指摘の推奨事項は既に満たされている形です。
前半で挙げていただいた 2 点についても、認識は一致しています。
- 最初の
hits=の grep が失敗し、後続のextra_patternsが正常終了しても、err_fileの内容は残るので検出される - リポジトリ間の持ち越しは、各リポジトリの処理開始時に
: > "$err_file"でクリアすることで防いでいる
なお : > "$err_file" を置いた位置には、もう 1 つ理由があります。clone が >"$err_file" 2>&1 で同じファイルに書くため、clone が成功しても何か出力していた場合にそれが残ります。grep の判定を始める前にクリアしないと、clone の出力を grep の失敗と誤認します。
コード変更は不要と判断しました。
|
|
||
| if [ -s "$err_file" ]; then | ||
| errors=$((errors + 1)) | ||
| log " ERROR: ${name} — grep が失敗した: $(tr '\n' ' ' < "$err_file" | cut -c1-200)" |
There was a problem hiding this comment.
ℹ️ [LOW] $(tr '\n' ' ' < "$err_file" | cut -c1-200) はコマンド置換内でファイルを読み込んでいますが、err_file に制御文字や特殊なバイト列が含まれる場合（悪意あるリポジトリ名やファイル名など）、ログ出力が意図しない形式になる可能性があります。実害は限定的ですが、cat "$err_file" | head -c 200 のように単純化するか、出力をクォートで保護することを検討してください。現状の log 関数がどのように実装されているかにも依存しますが、ログインジェクションのリスクとして認識しておくべきです。
There was a problem hiding this comment.
据え置きます。 理由は、この書き方が既存の ERROR 行と揃えたものだからです。
同じスクリプトの clone 失敗が、以前から同じ形を使っています。
log " ERROR: ${name} — clone できなかった: $(tr '\n' ' ' < "$err_file")"今回の行はこれに cut -c1-200 を足しただけです（grep のエラーは複数ファイル分が並んで長くなりうるため）。ここだけ制御文字を落とすと、同じファイル内の同種の 2 行で扱いが分かれます。 対処するならスクリプト全体、さらには audit-repo-protection.sh を含めた方針として決めるべきものです。
脅威モデルについても、実害は限定的だと考えています。
- 監査対象は
config/texlive-tag-refs.jsonに列挙した smkwlab org の 12 リポジトリで、ファイル名は自分たちが管理している err_fileに入るのは grep / xargs / gh のエラーメッセージで、任意の入力ではないlog()はecho "[$(date ...)] $1"で、-eを付けていないためバックスラッシュ列は解釈されない
残るリスクは、追跡ファイル名に生の ANSI エスケープを含むリポジトリを監査対象に加えた場合に CI ログの表示が乱れることですが、この監査は org 内の自リポジトリを対象にしたもので、そこまでの入力を想定していません。
もし方針として制御文字を落とすなら、tr -d '\000-\010\013\014\016-\037' のように範囲を限る必要があります。[:print:] で絞ると UTF-8 の継続バイトが巻き込まれ、日本語のファイル名やエラーメッセージが壊れます。このリポジトリのログは日本語なので、ここは慎重に決める必要があります。
レビュー対応MEDIUM 1 件・LOW 1 件、いずれも確認のうえ据え置きました。コード変更はありません。CI は MEDIUM:
|
1 participant
Resolves #173
監査が grep の標準エラーを捨てていたため、起動すらしなかった grep と、何も見つけなかった grep が同じ結果になっていました。前者でもそのリポジトリは
okと報告されます。なぜ終了コードでは判定できないか
xargsが両者を同じ値に潰します。 一方 grep は、マッチが無いだけなら標準エラーに何も書きません。その有無が唯一の判定材料です。変更
既定検出と
extra_patternsの grep をどちらも既存のerr_fileに追記させ、両方が終わった時点で 1 回だけ判定します。errorsカウンタとERROR:行は clone 失敗が既に使っている枠組みで、そこに乗せています。実測で検証しました
BSD grep（
-P非対応）を意図的に呼び、3 つのケースを確認しました。③ が重要で、マッチなしを異常と誤判定しないことを確認しています。
設計思想は既にこのファイルにありました
同じスクリプトの
cdにこう書かれています。audit-repo-protection.shにも同趣旨のコメントがあります。考え方は共有されていて、grep の呼び出しにだけ及んでいませんでした。補足: #173 の当初の記述は誤りでした
この issue は当初「macOS で動かない（
xargs -rが BSD にない）」として起票しましたが、xargs -rは macOS でも動きます。また PCRE 非対応環境はスクリプト冒頭のチェックが正しく弾きます（実際 macOS で実行するとgrep が PCRE (-P) に対応していない。GNU grep が要るで停止します）。スクリプトのガードは設計どおり機能しており、残る問題は標準エラーの握り潰しだけでした。 issue 本文はその 1 点に絞って書き直してあります。